Skip to content

Comments

Added main functionality#1

Open
skippikp wants to merge 3 commits intomainfrom
changes
Open

Added main functionality#1
skippikp wants to merge 3 commits intomainfrom
changes

Conversation

@skippikp
Copy link
Owner

No description provided.

@vkhalikov vkhalikov self-requested a review January 17, 2022 11:53
@vkhalikov
Copy link
Collaborator

Замечания по работе калькулятора:

  • калькулятор почему-то работает в разобранном виде
  • при нажатии на клавиши операторов число на дисплее превращается в 0, я аж сначала подумал, что сломалось, и в целом из-за этого неудобно им пользоваться
  • если нажать на кнопку оператора при уже нажатой кнопке оператора, то всё сбрасывается
  • в собранном виде происходят странные штуки с драг-н-дропом, зоны не должны подсвечиваться, если нельзя переносить
20220119_135332.mp4
  • торчит :)
    image

@vkhalikov
Copy link
Collaborator

В целом, неплохо для первой самостоятельной работы, основное реализовал!

Copy link
Collaborator

@vkhalikov vkhalikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом, импрувишься, лайк!

Copy link
Collaborator

@vkhalikov vkhalikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Леетсс гоооу

return (
<div className="display">
<div className="display_text-field">
{currentOperand?.length > 11 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Немного "грязновато" написано, попробуй вынести вот эту логику среза в отдельную функцию.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitNumber должен вычисляться только если currentOperand?.length > 11, а сейчас вычисляется всегда, даже когда он может быть не нужен.
Поэтому я предложил сделать функцию, вынести за пределы компонента.
Можно назвать её shortenOperand или truncateOperand. Тогда вся запись будет логично читаться, сравни:
"если текущий операнд длиннее 11 - показать большое число, иначе - текущий операнд или 0"
"если текущий операнд длиннее 11 - укоротить операнд, иначе - текущий операнд или 0"

box-shadow: 0 3px 2px rgba(0,0,0,0.2);
}

.numbers_0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Интересно придумал :)

draggable={draggable}
onDoubleClick={onDblClick}
>
{React.Children.map(children, (child) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А почему именно так сделал?
Почему просто {children} не отрендерить?
Есть какие-то общие props, которые хочешь через контейнер передавать сразу всех потомкам?

import { connect } from 'react-redux';

const Button = ({ constructorModeEnable, className, value, onClick }) => {
const clickHandler = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не стоит такую конкретику в такие общие компоненты прописывать.
Вот если бы компонент назывался CalculatorButton... :)


const rootReducer = combineReducers({
calc: calcReducer,
drag: constructorReducer,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drag -> constructor?

};
case DRAG_START:
if (state.draggableBlock === payload) {
return { ...state };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

можно просто state вернуть, зачем новый объект создаёшь?

import { connect } from 'react-redux';
import './Operations.css';

const OPERATIONS = ['/', 'x', '-', '+'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У тебя, вроде бы, между константами отступ в одну строку. Постоянство должно быть везде.
Обычно на проектах для этого используют линтеры, такие как eslint.
Попробуй в следующем проекте использовать линтер.

return (
<div className="display">
<div className="display_text-field">
{currentOperand?.length > 11 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitNumber должен вычисляться только если currentOperand?.length > 11, а сейчас вычисляется всегда, даже когда он может быть не нужен.
Поэтому я предложил сделать функцию, вынести за пределы компонента.
Можно назвать её shortenOperand или truncateOperand. Тогда вся запись будет логично читаться, сравни:
"если текущий операнд длиннее 11 - показать большое число, иначе - текущий операнд или 0"
"если текущий операнд длиннее 11 - укоротить операнд, иначе - текущий операнд или 0"

setDroppableSection(null);
};

const renderElement = (element) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Эта логика повторяется и в компоненте CalcPalette. Стоит вынести в отдельную функцию/компонент.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants